Copilot supported conversion to SCSS Modules#13013
Copilot supported conversion to SCSS Modules#13013andrewscfc wants to merge 101 commits intolatestfrom
Conversation
| "puppeteer": "24.10.2", | ||
| "retry": "0.13.1", | ||
| "sass": "1.89.2", | ||
| "sass-loader": "16.0.5", |
There was a problem hiding this comment.
copilot recommended packages to support SCSS
| @@ -0,0 +1,223 @@ | |||
| // fontFaces.module.scss | |||
| // SCSS mixins for @font-face rules, matching the fontFaces.ts constants | |||
| // Usage: @include reith-serif-regular-face; etc. | |||
There was a problem hiding this comment.
seems a pretty direct conversion from the ts constants: https://github.com/bbc/simorgh/blob/latest/src/app/components/ThemeProvider/fontFaces.ts
| @@ -0,0 +1,22 @@ | |||
| // fontFamilies.module.scss | |||
| // SCSS variables for font-family stacks, matching fontFamilies.ts | |||
| // Usage: font-family: $reith-sans; | |||
There was a problem hiding this comment.
| @@ -0,0 +1,14 @@ | |||
| // fontMediaQueries.module.scss | |||
| // SCSS variables for font media query breakpoints, matching fontMediaQueries.ts | |||
| // These are static values representing only the media query condition (no @media, no quotes). | |||
There was a problem hiding this comment.
After some back and forth with Copilot it told me that you can't have constants that contain full media queries like the original file
You can get very close though with the condition being encapsulating in a variable
| $values: map-get(map-get($font-sizes, $scale), $group); | ||
| font-size: map-get($values, font-size); | ||
| line-height: map-get($values, line-height); | ||
| } |
There was a problem hiding this comment.
This was a really difficult conversion, the theme dictates the font size and line height so it seemed sensible for these be declared as global variables to set at runtime by JS based on the service theme. In theory the algorithm otherwise works the same as the ts version: https://github.com/bbc/simorgh/blob/latest/src/app/components/ThemeProvider/fontSizes.ts
| $storm: #404040; | ||
| $success-core: #148A00; | ||
| $weather-blue: #067EB3; | ||
| $white: #FFFFFF; |
There was a problem hiding this comment.
| $margin-below-400px: $full; | ||
| $gutter-below-600px: $full; | ||
| $margin-above-400px: $double; | ||
| $gutter-above-600px: $double; |
There was a problem hiding this comment.
Quite a good comparison to the just CSS Modules equivalent, pretty much all the imperative logic is retained from the original: https://github.com/bbc/simorgh/blob/latest/src/app/components/ThemeProvider/spacings.ts
| 'brand-logo': 'var(--white)', | ||
| 'brand-foreground': 'var(--ghost)', | ||
| 'brand-highlight': 'var(--white)', | ||
| 'brand-border': 'var(--postbox-30)', |
There was a problem hiding this comment.
First example pulling in custom properties dynamically based on theme, this should reflect current behaviour
c3a0d1d to
8821093
Compare
8821093 to
0a8dcd6
Compare
| "jest-serializer-html": "7.1.0", | ||
| "jest-silent-reporter": "0.6.0", | ||
| "jsdom": "26.1.0", | ||
| "mini-css-extract-plugin": "2.9.2", |
There was a problem hiding this comment.
Copilot claims this enables extraction of css into seperate chunks based on usage
| $ratio: if($cropped-height != 0, $cropped-width / $cropped-height, 1); | ||
|
|
||
| :root { | ||
| --brand-logo-ratio: #{$ratio}; |
There was a problem hiding this comment.
It's a bit of a stretch but in principle this algorithm will set this custom css property to refer to when putting the css together for the brand here:
| @@ -0,0 +1,30 @@ | |||
| @mixin build-logo( | |||
There was a problem hiding this comment.
This meant to be roughly functional equivalent to: https://github.com/bbc/simorgh/blob/latest/src/app/components/ThemeProvider/chameleonLogos/index.tsx#L5
it compute values to use elsewhere for the brand and provides a suitable css class to use for styles
| stroke: #000; | ||
| stroke-width: 0.335; | ||
| /* Optionally, set stroke via custom property or theme */ | ||
| } No newline at end of file |
There was a problem hiding this comment.
I think this needs moving to the :root declaration above to make it global for use elsewhere
|
|
||
| .mundoLogoTheme { | ||
| @include logo.build-logo(1738, 425); | ||
| } |
There was a problem hiding this comment.
Applies mundo specific config to logo and actually executes the function, adding the suitably populated custom properties to the page
| import '../../../fontVariants/reith.module.scss'; | ||
| import './palette.module.scss'; | ||
| import '../../../fontScripts/latinWithDiacritics.module.scss'; | ||
| import '../../../chameleonLogos/mundoLogo.module.scss'; |
There was a problem hiding this comment.
Quite important implementation detail here, these scss modules are imported and applied to the page, but because this is dynamically imported via loadable the theory is this css is correctly chunked with mundo only i.e. only css for the chosen theme is loaded
| :root { | ||
| --brand-background: var(--postbox); | ||
| --brand-logo: var(--white); | ||
| --brand-foreground: var(--ghost); | ||
| --brand-highlight: var(--white); | ||
| --brand-border: var(--postbox-30); | ||
| } |
There was a problem hiding this comment.
| :root { | |
| --brand-background: var(--postbox); | |
| --brand-logo: var(--white); | |
| --brand-foreground: var(--ghost); | |
| --brand-highlight: var(--white); | |
| --brand-border: var(--postbox-30); | |
| } |
This file was moved, should be deleted
| :root { | ||
| --brand-background: var(--postbox); | ||
| --brand-logo: var(--white); | ||
| --brand-foreground: var(--ghost); | ||
| --brand-highlight: var(--white); | ||
| --brand-border: var(--postbox-30); | ||
| } |
There was a problem hiding this comment.
Equivalent of:
| }; | ||
|
|
||
|
|
||
| export default ThemeProvider; No newline at end of file |
There was a problem hiding this comment.
Equivalent of
This is simplified for now and obviously creates its own Provider rather than use emotion
| }; | ||
| }; | ||
|
|
||
| export default buildLogo; |
There was a problem hiding this comment.
Existing logic for brand svg unchanged by move to SCSS modules
| import '../../../fontFaces/reith-serif-light.module.scss'; | ||
| import '../../../fontVariants/reith.module.scss'; | ||
| import './palette.module.scss'; | ||
| import '../../../fontScripts/latinWithDiacritics.module.scss'; |
There was a problem hiding this comment.
Quite important implementation detail here, these scss modules are imported and applied to the page, but because this is dynamically imported via loadable the theory is this css is correctly chunked with mundo only i.e. only css for the chosen theme is loaded
| import '../../fontFaces/reith-serif-light.module.scss'; | ||
| import '../../fontVariants/reith.module.scss'; | ||
| import './palette.module.scss'; | ||
| import '../../fontScripts/latinWithDiacritics.module.scss'; |
There was a problem hiding this comment.
Quite important implementation detail here, these scss modules are imported and applied to the page, but because this is dynamically imported via loadable the theory is this css is correctly chunked with mundo only i.e. only css for the chosen theme is loaded
403d74b to
a5a4f48
Compare
| .h2 { | ||
| font-family: var(--sans-bold-font-family); | ||
| font-style: var(--sans-bold-font-style); | ||
| font-weight: var(--sans-bold-font-weight); |
There was a problem hiding this comment.
Font values are set from those applied here: https://github.com/bbc/simorgh/pull/13013/files#diff-606b2cf1ad95cb660e27b11c09db812ce4f721759062e7cb05c0488e8e892f01R5
There was a problem hiding this comment.
This usage font variables should be abstracted by a mixin, todo
Co-authored-by: Andrew Bennett <andrew.bennett07@bbc.co.uk>
| import '../../fontFaces/reith-serif-light.scss'; | ||
| import '../../fontVariants/reith.scss'; | ||
| import './palette.scss'; | ||
| import '../../fontScripts/latinWithDiacritics.scss'; |
There was a problem hiding this comment.
These imports all have side-effects, defining various css global variables specific to the theme. These are used to alter styles by theme dynamically at runtime.
A quite crucial bit of config was needed here to allow these side effects to take effect.
| mundo: loadable( | ||
| () => import(/* webpackChunkName: "themes-mundo" */ './mundo/mundo'), | ||
| ), | ||
| }; |
There was a problem hiding this comment.
The same dynamic import approach is utilised for themes; this allows only the relevant assets to be requested as before.
| $group-3-max-width-bp: 62.9375; // 1007px / 16 | ||
| $group-4-min-width-bp: 63; // 1008px / 16 | ||
| $group-4-max-width-bp: 79.9375; // 1279px / 16 | ||
| $group-5-min-width-bp: 80; // 1280px / 16 |
There was a problem hiding this comment.
Should be changed to use this utility for better readability to mirror the original https://github.com/bbc/simorgh/pull/13013/changes#diff-540a8d34d9211a4ca979d5eaef6394b41db38f899063a5519636507f5a9b0113R3
| $triple: $full * 3; // 1.5rem (24px) | ||
| $quadruple: $full * 4; // 2rem (32px) | ||
| $quintuple: $full * 5; // 2.5rem (40px) | ||
| $sextuple: $full * 6; // 3rem (48px) |
There was a problem hiding this comment.
Should be changed to use this utility for better readability to mirror the original https://github.com/bbc/simorgh/pull/13013/changes#diff-540a8d34d9211a4ca979d5eaef6394b41db38f899063a5519636507f5a9b0113R3
| {RenderChildrenOrError} | ||
| </PageWrapper> | ||
| </ThemeProvider> | ||
| <ThemeProviderSCSSModules service="mundo"> |
There was a problem hiding this comment.
hardcode to mundo for now
| modules: true, | ||
| importLoaders: 1, | ||
| esModule: false, | ||
| }, |
There was a problem hiding this comment.
This config was very trial and error to achieve, copilot suggests that:
modules: trueis enabled to support css modulesimportLoaders: 1ensures files imported in css modules go through the sass loader firstesModule: falseis needed for compatibility with style loader
This may be an area to optimise but with Express going soon it isn't a major area of concern
| use: ['css-loader', 'sass-loader'], | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
same config as client except style-loader/MiniCssExtractPlugin is not needed as the css is not being inserted into pages on the server
…ontVariants/index.tsx [copilot]
Summary
This PR presents a viable alternative to Emotion for styling; in a retro in the summer we identified that Emotion presents several pain points and currently blocks the adoption of App Router in NextJS.
The PR presents an approach utilising SASS, CSS Modules and CSS custom variables; the high level approach is as follows:
ThemeProviderSCSSModulesthat imports relevant fonts and styles dependent on the chosen service, as theThemeProviderdoes todayThemeProviderSCSSModulesimports css modules that declare suitable css custom variables that are dictated by the chosen serviceCuration/Subheadis implemented as an example utilising ths new theme system with SCSS\ArticlesLinksBlockis a bigger component implemented with the new theme system.font-sizeandline-heightas controlled by the css custom variables configured by the theme.Tech Choice Rationale
Adoption path
Limitations
Next Steps
Curation/Subhead+ArticleLinksBlockonlyUseful Links